fix(tests): the RSS gate could report unfinished warmup as a leak (#765) - #767
Conversation
The gate warmed up, took checkpoint A, drove a measured batch, took B, and
judged B-A. That is only valid if the warmup actually reached steady state —
and a single warmup-then-measure cannot tell whether it did. When it hasn't,
B-A re-absorbs the ~1.4 MB of one-time arena growth the two-checkpoint method
exists to exclude, and reports it as a per-request leak.
That is what the intermittent db-job failures were. Forcing WARMUP=5 on a
leak-free current binary reproduces the CI figures to within ~1% on BOTH checks:
reported RSS1 2852 kB / 1460 B-per-req RSS2 1516 kB / 776 B-per-req
repro RSS1 2880 kB / 1474 B-per-req RSS2 1556 kB / 796 B-per-req
Counting completed requests is not sufficient on its own: WARMUP=5 completes
5/5 and is still wrong. A batch that was *truncated* and a batch that was merely
*too short for this machine* produce an identical signature, which is why a
slower, contended runner (the db job is the only one combining the full variant,
a container, and a live postgres service) failed ~25% of the time while the same
gate ran clean locally on every attempt.
The discriminator is physical, not statistical: one-time warmup DECAYS, a
per-request leak does not. A leak of N bytes/req leaks the same N in every
later batch, forever; arena warmup is spent and does not recur. So drive TWO
identical measured batches and judge the SECOND — by then WARMUP + MEASURE
requests have run, so any warmup tail is behind us, and a real leak is entirely
unaffected by which batch it is measured in. The first batch is still reported
when it disagrees, because that disagreement is the fingerprint of an
unfinished warmup and is worth seeing before someone re-diagnoses it as a leak.
Batch completion is now also asserted, and an incomplete batch fails as an
INSTRUMENT error by that name rather than silently shortening the interval a
checkpoint bounds.
The threshold is NOT loosened, by a single byte. Validated both ways against a
planted fault in handle_request:
- phantom: WARMUP=5 (first batch 2880/1556 kB, i.e. CI's own numbers) now
PASSES, logging "unfinished warmup, decayed as expected"
- real: a 256 B/req TOUCHED leak, ~1.6x the original #731 rate, still FAILS
both checks, logging "growth did NOT decay, so this is a leak, not warmup"
The touched part is load-bearing and cost me a wrong conclusion first: an
untouched malloc(256) per request is NOT detected, and correctly so — pages
that are mapped and never written are not resident, so they are not RSS. The
leaks this gate exists to catch (parsed JSON Values, strings) are all written.
A planted fault for this instrument must write to what it leaks.
This also clears 8aaf44a, which the report flagged as the bisection point: its
entire ext_http.c change is two NULL guards that fire only when
eigs_json_encode refuses, which a number never triggers. 52aa990 — a later main
commit containing it — passed the same job, which alone falsifies a
deterministic leak introduced there.
Release suite 3274/3274.
Closes #765
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Updates the HTTP RSS per-request leak gate to avoid misreporting unfinished warmup as a leak by measuring two consecutive batches and judging the second, while also validating that the intended number of requests actually returned HTTP 200.
Changes:
- Add
batch_200s()to count successful (200) responses for a curl-globbed batch and fail the check when batches don’t produce the expected number of 200s. - Change the RSS growth methodology to run two identical measured batches and use the second batch’s RSS delta as the verdict (with the first batch reported when it disagrees).
- Apply the same two-batch methodology to both RSS1 (
shared_incr) and RSS2 (authed route) checks, including improved logging for “unfinished warmup” vs “real leak” signatures.
Comments suppressed due to low confidence (1)
tests/test_http_rss_growth.sh:201
- Same issue as above: this check is validating the number of 200 responses, not strictly that the batch ran to completion. The error message should reflect that non-200 statuses (not just early termination) make the RSS deltas meaningless.
fail "RSS2 authed route INSTRUMENT: batch incomplete, RSS deltas are meaningless" \
"warmup $WARM/$WARMUP, batches $M1/$MEASURE and $M2/$MEASURE — not a leak measurement"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fail "$label INSTRUMENT: batch incomplete, RSS deltas are meaningless" \ | ||
| "warmup $warm/$WARMUP, batches $m1/$MEASURE and $m2/$MEASURE — not a leak measurement" |
Update: reproduced the actual flake locally, in the failing configuration
The flake needs the db build
So db linkage is the differentiating axis, as #765 suspected but could not test. Local rate (~10%) is lower than CI's ~25%, which is what you'd expect from a faster, less contended machine — the trigger is how long arena warmup takes relative to The two harnesses observing the same eventSame configuration, run 4 of each: 1332 kB and 1340 kB are the same overhang. The new harness sees it, reports it, and classifies it correctly — because the second batch decayed to 0 kB, which a per-request leak cannot do. The old one had no way to ask that question. Note the new harness does not hide the anomaly: when warmup hasn't finished it says so in the passing line, so the condition stays visible in CI logs rather than becoming invisible. Why the db build warms up longerNot separately proven, and I'm not going to assert a mechanism I haven't measured. Linking libpq pulls in more shared objects and changes allocator/arena behavior, which plausibly stretches the one-time warmup past 1000 requests. The fix does not depend on which explanation is right — it depends only on warmup decaying and a leak not decaying. Thanks for pushing on the libpq question — I had taken "this box has no libpq" from #765 as a fixed constraint instead of testing whether it had to be, and that was the wrong call. It was a one-line install away, and it turned a numerically-matched inference into a direct reproduction. |
Closes #765
It is the instrument, not a leak
The gate warmed up, took checkpoint A, drove a measured batch, took B, judged
B-A. That is only valid if the warmup actually reached steady state — and a single warmup-then-measure cannot tell whether it did. When it hasn't,B-Are-absorbs the ~1.4 MB of one-time arena growth that the two-checkpoint method exists to exclude, and reports it as a per-request leak.Forcing
WARMUP=5on a leak-free current binary reproduces the reported CI figures to within ~1% on both checks:Counting requests is not enough
My first attempt asserted batch completion. That is worth having and is included — but it does not fix this.
WARMUP=5completes 5/5 and is still wrong. A batch that was truncated and one merely too short for this machine produce an identical signature, which is why the db job — the only one combining the full variant, a container, and a live postgres service — failed ~25% of the time while the same gate ran clean locally on every attempt.The discriminator is physical
One-time warmup decays; a per-request leak does not. A leak of N bytes/req leaks the same N in every later batch, forever. Arena warmup is spent and does not recur.
So: drive two identical measured batches and judge the second. By then
WARMUP + MEASURErequests have run, so any warmup tail is behind us — and a real leak is entirely unaffected by which batch you measure it in. The first batch is still reported when it disagrees, since that disagreement is the fingerprint of an unfinished warmup and is worth seeing before someone re-diagnoses it as a leak.The threshold is not loosened by a single byte.
Validated both ways, against a planted fault in
handle_requestWARMUP=5, first batch 2880/1556 kB (CI's own numbers): now PASSES, loggingunfinished warmup, decayed as expectedgrowth did NOT decay, so this is a leak, not warmupOne finding worth keeping
My first planted fault was an untouched
malloc(256)per request, and the gate did not catch it. That is correct behavior, not a hole: pages mapped and never written are not resident, so they are not RSS. I nearly filed the gate as blind on that basis. The leaks this instrument exists to catch — parsed JSON Values, strings — are all written. A planted fault for this instrument must write to what it leaks, and that is now recorded in the file.This clears 8aaf44a
#765 named it as the bisection point, carefully, as correlation. Its entire
ext_http.cchange is two NULL guards that fire only wheneigs_json_encoderefuses — which a number never triggers, so they are inert onshared_incr. And52aa990, a later main commit that contains it, passed the same job, which on its own falsifies a deterministic leak introduced there.The clean 8-run before/after boundary was suggestive but not decisive: at a 25% rate, 8 consecutive passes has probability 0.75^8 ≈ 10%.
Release suite 3274/3274.
🤖 Generated with Claude Code